[Bug][API] Make kubectl get modeladapter show a truthful status - #2624
Conversation
Signed-off-by: Alex Jia <yj2761@nyu.edu>
Signed-off-by: Alex Jia <yj2761@nyu.edu>
Signed-off-by: Alex Jia <yj2761@nyu.edu>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the lifecycle phase and readiness status logic for ModelAdapter to ensure that the status accurately reflects the state of loaded instances, preventing premature 'Running' or 'Ready=True' states. It introduces a centralized readiness recomputation mechanism, updates CRD print columns, and updates the documentation. A comprehensive suite of unit tests is also added. The review feedback suggests refactoring the updateStatus logging logic to prevent misleading log messages when status changes are persisted.
| func (r *ModelAdapterReconciler) syncReadinessStatus(ctx context.Context, oldInstance, instance *modelv1alpha1.ModelAdapter) error { | ||
| recomputeReadiness(instance) | ||
| if !r.inconsistentModelAdapterStatus(oldInstance.Status, instance.Status) { | ||
| return nil | ||
| } | ||
| return r.updateStatus(ctx, instance) | ||
| } |
There was a problem hiding this comment.
This function correctly centralizes the status update logic. However, the call to r.updateStatus(ctx, instance) on line 1075 can lead to confusing logs. The updateStatus function logs a changed flag which will be false in this call path, even though the status is changing (as determined by inconsistentModelAdapterStatus).
To improve log clarity, consider a small refactor of the updateStatus function to make its logging more accurate. For example:
func (r *ModelAdapterReconciler) updateStatus(ctx context.Context, instance *modelv1alpha1.ModelAdapter, conditions ...metav1.Condition) error {
var conditionsChanged bool
for _, condition := range conditions {
if meta.SetStatusCondition(&instance.Status.Conditions, condition) {
conditionsChanged = true
}
}
klog.InfoS("model adapter reconcile", "Updating CR status", "instance", instance.Name, "conditionsChanged", conditionsChanged, "status", instance.Status)
return r.Status().Update(ctx, instance)
}This change clarifies that the logged flag refers specifically to whether the passed-in conditions caused a change, making debugging easier.
🧵 Code Review Comments🚨 Severe (Should fix before merge)1. Semantic clash between
On a ready-but-unstable pod,
2.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new recomputeReadiness currently makes the Scheduled phase effectively unobservable (it gets overwritten to Pending), conflicting with the documented lifecycle and single-pod scheduling behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the correctness and observability of kubectl get modeladapter by making the controller consistently recompute and persist readiness-related status fields, and by surfacing the Ready condition reason/message via CRD printer columns and docs.
Changes:
- Controller: adds
recomputeReadiness/syncReadinessStatusto deriveReadyReplicas,Phase, and theReadycondition from the final instance set and to avoid stale “Running/Ready” states. - API/CRD: adds
Reason(andBase Model/Instances/Messageas wide columns) to make non-ready causes visible inkubectl get. - Tests/docs/samples: adds controller tests for the new readiness behavior and updates lifecycle documentation and sample manifests.
File summaries
| File | Description |
|---|---|
| samples/adapter/adapter.yaml | Updates lifecycle phase documentation comment to match the new status model. |
| pkg/controller/modeladapter/modeladapter_status_test.go | Adds unit + fake-client tests covering readiness recomputation and reconcile scenarios. |
| pkg/controller/modeladapter/modeladapter_controller.go | Introduces readiness recomputation and sync logic; removes EndpointSlice-driven “force Running”. |
| docs/source/features/lora-dynamic-loading.rst | Updates lifecycle docs and adds example kubectl get outputs including reason/message. |
| dist/chart/crds/model.aibrix.ai_modeladapters.yaml | Adds printer columns (Reason/Base Model/Instances/Message) to the shipped CRD. |
| config/crd/model/model.aibrix.ai_modeladapters.yaml | Adds the same printer columns to the source CRD manifest. |
| api/model/v1alpha1/modeladapter_types.go | Adds kubebuilder printcolumn annotations for the new columns. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| switch { | ||
| case status.ReadyReplicas > 0: | ||
| status.Phase = modelv1alpha1.ModelAdapterRunning | ||
| meta.SetStatusCondition(&status.Conditions, NewCondition(string(modelv1alpha1.ModelAdapterConditionReady), metav1.ConditionTrue, | ||
| ModelAdapterAvailable, fmt.Sprintf("ModelAdapter %s is ready", klog.KObj(instance)))) | ||
| case status.Phase == modelv1alpha1.ModelAdapterFailed: | ||
| // Keep the loading error recorded by reconcileLoading visible. | ||
| case status.Candidates == 0: | ||
| status.Phase = modelv1alpha1.ModelAdapterPending | ||
| meta.SetStatusCondition(&status.Conditions, NewCondition(string(modelv1alpha1.ModelAdapterConditionReady), metav1.ConditionFalse, | ||
| PodNotReadyReason, "no ready pods match the pod selector")) | ||
| default: | ||
| status.Phase = modelv1alpha1.ModelAdapterPending | ||
| meta.SetStatusCondition(&status.Conditions, NewCondition(string(modelv1alpha1.ModelAdapterConditionReady), metav1.ConditionFalse, | ||
| ModelAdapterUnavailable, fmt.Sprintf("adapter is not loaded on any of the %d candidate pods", status.Candidates))) | ||
| } |
|
@yaojiejia could you rebase the main branch? |
…dapter-status-columns Signed-off-by: Alex Jia <yj2761@nyu.edu>
just rebased |
Pull Request Description
kubectl get modeladapteralready had Phase/Desired/Ready/Candidates columns, but the controller often left them blank, stale or wrong, and nothing said why an adapter was not ready.Problems fixed
CandidatesandDesiredReplicaswere computed but never saved. The adapter showedPendingwith empty counters and no reason.Runningadapter lost all its pods,ReadyReplicaswas not recomputed and the EndpointSlice code re-setPhase=Running, sokubectl getshowedRunning Ready=1 Candidates=0.Changes
recomputeReadiness) now derivesReadyReplicas,Phaseand theReadycondition from the loaded instances at the end of every reconcile.Runningis only set when at least one instance is loaded. The "waiting for pods" path now saves the counters with aPodNotReadyorModelAdapterUnavailablereason.Failedis kept until a load succeeds. Status is written only when something changed.Reasoncolumn (from theReadycondition).Base Model,InstancesandMessageare-o widecolumns.DoReconciletests for the three scenarios above (they fail on the old controller).Loadingphase,Boundis not the success state) and shows samplekubectl getoutput.$ kubectl get modeladapter
NAME PHASE DESIRED READY CANDIDATES REASON MODEL PATH AGE
qwen-code-lora Pending PodNotReady hf://... 5s
qwen-code-lora Running 1 1 1 ModelAdapterAvailable hf://... 2m
Behavior changes to note
Pending/PodNotReadyright away instead of keeping the staleRunning. TheAdapterMigrating/Rescheduledconditions still appear when a replacement pod exists.Failedstays until a load succeeds, even if all pods are gone.Instancesrenders as a JSON list in-o wide.Testing
make manifests-all,make verify-crd,hack/verify-codegen.sh,make lint-all,go test ./pkg/controller/modeladapter/... ./api/...(with and without-race) all pass.Related Issues
Resolves: #238
Important: Before submitting, please complete the description above and review the checklist below.
Contribution Guidelines (Expand for Details)
We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]: Corrections to existing functionality[CI]: Changes to build process or CI pipeline[Docs]: Updates or additions to documentation[API]: Modifications to aibrix's API or interface[CLI]: Changes or additions to the Command Line Interface[Misc]: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.